[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

emaxx-google · 2025-06-18T21:39:30Z

Change the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes #143152.

This commit removes test assertions from the regression test of #108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix.

Change the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes llvm#143152. This commit removes test assertions from the regression test of llvm#108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix.

llvmbot · 2025-06-18T21:40:00Z

@llvm/pr-subscribers-clang

Author: Maksim Ivanov (emaxx-google)

Changes

Change the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes #143152.

This commit removes test assertions from the regression test of #108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix.

Full diff: https://github.com/llvm/llvm-project/pull/144796.diff

3 Files Affected:

(modified) clang/lib/AST/ODRHash.cpp (+17-1)
(modified) clang/test/Modules/odr_hash.cpp (+23)
(modified) clang/test/PCH/race-condition.cpp (+6-5)

diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index f8446dfbc6859..84fac90a03e87 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -828,7 +828,23 @@ void ODRHash::AddDecl(const Decl *D) {
     return;
   }
 
-  AddDeclarationName(ND->getDeclName());
+  // For template parameters, use depth+index instead of name, because type
+  // canonicalization can change the name of the template parameter.
+  if (auto *TTPD = dyn_cast<TemplateTypeParmDecl>(ND)) {
+    ID.AddInteger(TTPD->getDepth());
+    ID.AddInteger(TTPD->getIndex());
+    AddBoolean(TTPD->isParameterPack());
+  } else if (auto *NTTPD = dyn_cast<NonTypeTemplateParmDecl>(ND)) {
+    ID.AddInteger(NTTPD->getDepth());
+    ID.AddInteger(NTTPD->getIndex());
+    AddBoolean(NTTPD->isParameterPack());
+  } else if (auto *TTPD = dyn_cast<TemplateTemplateParmDecl>(ND)) {
+    ID.AddInteger(TTPD->getDepth());
+    ID.AddInteger(TTPD->getIndex());
+    AddBoolean(TTPD->isParameterPack());
+  } else {
+    AddDeclarationName(ND->getDeclName());
+  }
 
   // If this was a specialization we should take into account its template
   // arguments. This helps to reduce collisions coming when visiting template
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index da24b1fe8729a..8ef53e32f2e95 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -5164,6 +5164,29 @@ namespace A {
 A::X x;
 #endif
 
+namespace TemplateDecltypeOperator {
+
+#if defined(FIRST) || defined(SECOND)
+template <class T6>
+T6 func();
+#endif
+
+#if defined(SECOND)
+template <class UnrelatedT>
+using UnrelatedAlias = decltype(func<UnrelatedT>())();
+#endif
+
+#if defined(FIRST) || defined(SECOND)
+class A {
+  template <class T6>
+  operator decltype(func<T6>()) () {}
+};
+#else
+A a;
+#endif
+
+}
+
 // Keep macros contained to one file.
 #ifdef FIRST
 #undef FIRST
diff --git a/clang/test/PCH/race-condition.cpp b/clang/test/PCH/race-condition.cpp
index 752b0cc3ff628..666d92ed2ce8b 100644
--- a/clang/test/PCH/race-condition.cpp
+++ b/clang/test/PCH/race-condition.cpp
@@ -31,11 +31,12 @@ constexpr enable_if_t<meta<F>::value == 2, void> midpoint(F) {}
 
 #else
 
-// expected-error@27{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'F'}}
-// expected-error@24{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'U'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// FIXME: Change the test to trigger a suitable error: previously the test
+// asserted the ODR error ("'N::midpoint' has different definitions in different
+// modules"), which isn't fully correct as there's only one module, and since a
+// change in the ODR hash calculation this error isn't triggered anymore.
+
 int x = N::something;
-// expected-error@37{{no member named 'something' in namespace 'N'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// expected-error@38{{no member named 'something' in namespace 'N'}}
 
 #endif

llvmbot · 2025-06-18T21:40:01Z

@llvm/pr-subscribers-clang-modules

Author: Maksim Ivanov (emaxx-google)

Changes

Change the ODR hashing logic to use the depth+index indices instead of template parameter names. This prevents spurious ODR errors in header module builds when the type canonicalization picks up different expressions - with different template parameter names (profiling used for canonicalization ignores template parameter names). This fixes #143152.

This commit removes test assertions from the regression test of #108866 - the compiler errors asserted by that test stop firing with this commit. It's questionable whether these diagnostics were correct. We add a FIXME to find a better regression test for that fix.

Full diff: https://github.com/llvm/llvm-project/pull/144796.diff

3 Files Affected:

(modified) clang/lib/AST/ODRHash.cpp (+17-1)
(modified) clang/test/Modules/odr_hash.cpp (+23)
(modified) clang/test/PCH/race-condition.cpp (+6-5)

diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp
index f8446dfbc6859..84fac90a03e87 100644
--- a/clang/lib/AST/ODRHash.cpp
+++ b/clang/lib/AST/ODRHash.cpp
@@ -828,7 +828,23 @@ void ODRHash::AddDecl(const Decl *D) {
     return;
   }
 
-  AddDeclarationName(ND->getDeclName());
+  // For template parameters, use depth+index instead of name, because type
+  // canonicalization can change the name of the template parameter.
+  if (auto *TTPD = dyn_cast<TemplateTypeParmDecl>(ND)) {
+    ID.AddInteger(TTPD->getDepth());
+    ID.AddInteger(TTPD->getIndex());
+    AddBoolean(TTPD->isParameterPack());
+  } else if (auto *NTTPD = dyn_cast<NonTypeTemplateParmDecl>(ND)) {
+    ID.AddInteger(NTTPD->getDepth());
+    ID.AddInteger(NTTPD->getIndex());
+    AddBoolean(NTTPD->isParameterPack());
+  } else if (auto *TTPD = dyn_cast<TemplateTemplateParmDecl>(ND)) {
+    ID.AddInteger(TTPD->getDepth());
+    ID.AddInteger(TTPD->getIndex());
+    AddBoolean(TTPD->isParameterPack());
+  } else {
+    AddDeclarationName(ND->getDeclName());
+  }
 
   // If this was a specialization we should take into account its template
   // arguments. This helps to reduce collisions coming when visiting template
diff --git a/clang/test/Modules/odr_hash.cpp b/clang/test/Modules/odr_hash.cpp
index da24b1fe8729a..8ef53e32f2e95 100644
--- a/clang/test/Modules/odr_hash.cpp
+++ b/clang/test/Modules/odr_hash.cpp
@@ -5164,6 +5164,29 @@ namespace A {
 A::X x;
 #endif
 
+namespace TemplateDecltypeOperator {
+
+#if defined(FIRST) || defined(SECOND)
+template <class T6>
+T6 func();
+#endif
+
+#if defined(SECOND)
+template <class UnrelatedT>
+using UnrelatedAlias = decltype(func<UnrelatedT>())();
+#endif
+
+#if defined(FIRST) || defined(SECOND)
+class A {
+  template <class T6>
+  operator decltype(func<T6>()) () {}
+};
+#else
+A a;
+#endif
+
+}
+
 // Keep macros contained to one file.
 #ifdef FIRST
 #undef FIRST
diff --git a/clang/test/PCH/race-condition.cpp b/clang/test/PCH/race-condition.cpp
index 752b0cc3ff628..666d92ed2ce8b 100644
--- a/clang/test/PCH/race-condition.cpp
+++ b/clang/test/PCH/race-condition.cpp
@@ -31,11 +31,12 @@ constexpr enable_if_t<meta<F>::value == 2, void> midpoint(F) {}
 
 #else
 
-// expected-error@27{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'F'}}
-// expected-error@24{{'N::midpoint' has different definitions in different modules; defined here first difference is 1st parameter with type 'U'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// FIXME: Change the test to trigger a suitable error: previously the test
+// asserted the ODR error ("'N::midpoint' has different definitions in different
+// modules"), which isn't fully correct as there's only one module, and since a
+// change in the ODR hash calculation this error isn't triggered anymore.
+
 int x = N::something;
-// expected-error@37{{no member named 'something' in namespace 'N'}}
-// expected-note@21{{but in '' found 1st parameter with type 'T'}}
+// expected-error@38{{no member named 'something' in namespace 'N'}}
 
 #endif

emaxx-google · 2025-06-18T21:41:18Z

clang/test/Modules/odr_hash.cpp

+#endif
+
+#if defined(FIRST) || defined(SECOND)
+class A {


Without the changes in this PR, this test would fail with this message:

'TemplateDecltypeOperator::A' with definition in module 'SecondModule' has different definitions in different modules; first difference is this function template
but in 'FirstModule' found different function template

Could you add a comment to this test that points to this PR/GitHub issue and mentions that we need to make sure despite the fact that decltype(e) gets canonicalized to decltype(e_0) where e_0 can potentially have different template parameter names. Therefore, we need to ensure their ODR hashes are the same across multiple modules?

It might prove helpful in the future if somebody decides to reassess the choices made here and choose different trade-offs.

ilya-biryukov

Thanks, I believe we've seen some related breakages in the past and had to workaround the module structure because of this. It would definitely save some cycles for us in the future!

My only request is to keep this change focused on fixing the particular issue and split the variadics into a separate one.

ilya-biryukov · 2025-06-23T08:36:18Z

clang/test/Modules/odr_hash.cpp

+#endif
+
+#if defined(FIRST) || defined(SECOND)
+class A {


Could you add a comment to this test that points to this PR/GitHub issue and mentions that we need to make sure despite the fact that decltype(e) gets canonicalized to decltype(e_0) where e_0 can potentially have different template parameter names. Therefore, we need to ensure their ODR hashes are the same across multiple modules?

It might prove helpful in the future if somebody decides to reassess the choices made here and choose different trade-offs.

ilya-biryukov · 2025-06-23T08:50:33Z

clang/lib/AST/ODRHash.cpp

+  if (auto *TTPD = dyn_cast<TemplateTypeParmDecl>(ND)) {
+    ID.AddInteger(TTPD->getDepth());
+    ID.AddInteger(TTPD->getIndex());
+    AddBoolean(TTPD->isParameterPack());


Could you split the variadic templates into a separate change?

This looks like an independent issue (the code below should detect an ODR violation, but it doesn't) and actually opens a whole other can of worms wrt to what needs to be checked (e.g. checking types of non-type template parameters, template headers of template template parameters, etc).

namespace mytest { #ifdef FIRST template <class ...T> struct FooTypePack {}; template <int ...I> struct FooIntPack {}; template <template<class> class ...TT> struct FooTemplateTemplatePack {}; #endif #ifdef SECOND template <class T> struct FooTypePack {}; template <int I> struct FooIntPack {}; template <template<class> class TT> struct FooTemplateTemplatePack {}; #endif #ifdef ACCESS FooTypePack<int> a; FooIntPack<0> b; FooTemplateTemplatePack<FooTypePack> c; #endif }

mizvekov

The fix doesn't look quite right. It's not correct to say that canonicalization changes the declaration name. We don't canonicalize declarations after all. It's just that a canonical template parameter type doesn't refer to a particular template parameter declaration.

This change weakens the ODR checker beyond what the fix should require.

I can't give a more thorough review at this time since I am still traveling from the WG21 meeting.

ilya-biryukov · 2025-06-23T09:18:58Z

It's just that a canonical template parameter type doesn't refer to a particular template parameter declaration.

The particular statement is that Clang canonicalizes decltype(func<T>()) to decltype(func<FirstOccurrenceOfTWithSameDepthAndIndex>()). If we don't change that, we end up with spurious false positives from ODR checking (I believe that's one of the reasons why it's disabled in C++20 header units), effectively turning off all the checks and not just weakening them for template parameters.

I feel that this change is a better trade-off, at least that's the best idea for tactically reducing the false positive rate (at the cost of some false negatives).

This change weakens the ODR checker beyond what the fix should require.
I can't give a more thorough review at this time since I am still traveling from the WG21 meeting.

👍 eager to hear better suggestions and safe travels.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Jun 18, 2025

emaxx-google commented Jun 18, 2025

View reviewed changes

fix assert line number

4f800c8

ilya-biryukov self-requested a review June 23, 2025 08:36

ilya-biryukov requested changes Jun 23, 2025

View reviewed changes

mizvekov requested changes Jun 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

Uh oh!

emaxx-google commented Jun 18, 2025

Uh oh!

llvmbot commented Jun 18, 2025

Uh oh!

llvmbot commented Jun 18, 2025

Uh oh!

emaxx-google Jun 18, 2025

Uh oh!

ilya-biryukov Jun 23, 2025

Uh oh!

ilya-biryukov left a comment

Uh oh!

ilya-biryukov Jun 23, 2025

Uh oh!

ilya-biryukov Jun 23, 2025

Uh oh!

mizvekov left a comment

Uh oh!

ilya-biryukov commented Jun 23, 2025

Uh oh!

Uh oh!

[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

Are you sure you want to change the base?

[clang] ODR hashes depth+index and not name of TemplateTypeParm #144796

Uh oh!

Conversation

emaxx-google commented Jun 18, 2025

Uh oh!

llvmbot commented Jun 18, 2025

Uh oh!

llvmbot commented Jun 18, 2025

Uh oh!

emaxx-google Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov left a comment

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

mizvekov left a comment

Choose a reason for hiding this comment

Uh oh!

ilya-biryukov commented Jun 23, 2025

Uh oh!

Uh oh!